Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GitHub Organization Provider #5194

Merged
merged 2 commits into from
Mar 17, 2016

Conversation

johnrengelman
Copy link
Contributor

This adds a new provider for managing Github organizations. It provides resources for managing membership in the organization, organization teams, team membership, and team repositories.

The acceptance tests require the GITHUB_TOKEN and GITHUB_ORGANIZATION envvars to be configured before executing.

This provider covers #1731

@johnrengelman
Copy link
Contributor Author

Not sure what we need to do to add the new dependency. Guidance?

@jen20
Copy link
Contributor

jen20 commented Feb 18, 2016

@johnrengelman you should be able to do: godep save $(go list ./... | grep -v vendor) in order to update the dependencies. I believe newer versions of godep negate the need for that though.

Also could you consider squashing this into one commit and force pushing back to the PR branch?

@johnrengelman
Copy link
Contributor Author

@jen20 - will do. Getting on a plane atm, will update it tonight and figure out the deps.

@johnrengelman
Copy link
Contributor Author

@jen20 rebased & squashed.
I ran the godep command and it vendored the github library, but it also deleted some aws-sdk-go library...not sure why.

@johnrengelman
Copy link
Contributor Author

This breaks the build...why is there not a Makefile target for doing this?

@johnrengelman
Copy link
Contributor Author

I just did some manual tweaking of the import stuff to get it correct. @phinze discovered an outstanding issue in godeps that is causing the issue:
tools/godep#416
tools/godep#415

CI built successfully now.

@johnrengelman
Copy link
Contributor Author

Rebased and resolved the merge conflicts with the vendor changes.

johnrengelman and others added 2 commits March 8, 2016 23:06
Allows for managing organization membership, teams, team membership, and
team repositories.
@johnrengelman
Copy link
Contributor Author

Rebased this commit again and split out the vendor dependency to its own commit to facilitate review.

@phinze
Copy link
Contributor

phinze commented Mar 17, 2016

Got the tests passing on our side:

=== RUN   TestProvider
--- PASS: TestProvider (0.00s)
=== RUN   TestProvider_impl
--- PASS: TestProvider_impl (0.00s)
=== RUN   TestAccGithubMembership_basic
--- PASS: TestAccGithubMembership_basic (2.79s)
=== RUN   TestAccGithubTeamMembership_basic
--- PASS: TestAccGithubTeamMembership_basic (5.21s)
=== RUN   TestAccGithubTeamRepository_basic
--- PASS: TestAccGithubTeamRepository_basic (3.89s)
=== RUN   TestAccCheckGetPermissions
--- PASS: TestAccCheckGetPermissions (0.00s)
=== RUN   TestAccGithubTeam_basic
--- PASS: TestAccGithubTeam_basic (2.88s)
=== RUN   TestAccGithubUtilRole_validation
--- PASS: TestAccGithubUtilRole_validation (0.00s)
=== RUN   TestAccGithubUtilTwoPartID
--- PASS: TestAccGithubUtilTwoPartID (0.00s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/github 14.788s

Great work on this @JacobASeverson and @johnrengelman! Thank you!

phinze added a commit that referenced this pull request Mar 17, 2016
@phinze phinze merged commit 07caec0 into hashicorp:master Mar 17, 2016
@johnrengelman
Copy link
Contributor Author

@phinze you might want to look at the acceptance tests... @JacobASeverson just told me that that the account it uses for joining and stuff is hard coded...so maybe that should be a configurable option for spec.

@JacobASeverson
Copy link
Contributor

Yup realized that when I got an email flood from Github while you were doing testing. I can work on that later today.

@phinze
Copy link
Contributor

phinze commented Mar 17, 2016

This is just TerraformDummyUser yeah? I noticed that but figured it was clearly an account created for these tests and it worked so it seemed okay. Definitely would be a bit nicer to have that parameterized though!

@JacobASeverson
Copy link
Contributor

Yeah just a dummy account so won't hurt anything.

@ghost
Copy link

ghost commented Apr 27, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants